fix(http): release parsed JSON Values on three request paths (#731) - #754
Merged
Conversation
#731 reported shared_incr. Auditing every eigs_json_parse_value call site in ext_http.c found three of five leaking, two of them unreported: - shared_incr dropped TWO Values per call: the parsed counter on both the success path and the type-mismatch early return (the return an audit most easily misses), plus the make_num handed to eigs_json_encode, which borrows its argument rather than taking ownership. - builtin_http_post never released the parsed headers object. Released immediately after the header loop, not at function end, because the pipe/fork failure paths below it return through TRACE_NONDET_RECORD. - http_route_authed's shared-store auth branch never released the parsed require_auth string — a leak on every request to an authenticated route. shared_get transfers ownership to its caller and the :508 encode borrows; both are correct and untouched. Measured on a release build against a live server, steady-state batches: shared_incr 159 B/req -> 0, authenticated route 188 B/req -> 0. GATE: tests/test_http_rss_growth.sh, suite [45c]. RSS growth between two steady-state checkpoints — never baseline-to-end, since the first requests carry ~1.4 MB of one-time arena warmup, ~18x the real rate. No sanitizer can catch this class here: LeakSanitizer reports from atexit and the server is torn down with `kill` against no SIGTERM handler, so the ASan suite reported 32/32 green over this leak for as long as it existed. Validated by planting the fault back — both checks fail without the fix at the measured rates and pass with it. The gate skips on sanitizer builds. This is load-bearing, not caution: ASan's redzones and quarantine grow RSS 567 B/req on a FIXED binary, so running it in the asan-http job would have turned CI red on correct code. The suite prints the skip REASON rather than "PASS: all 0 checks", so a gate that never ran cannot read as coverage. Suite 3350/3350 on `make http` (gate active) and 3352/3352 under `make asan-http` with detect_leaks=1 (gate skipped). Closes #731 Closes #752 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes three per-request Value refcount leaks in the HTTP extension (ext_http.c) by correctly releasing JSON-parsed Values (and a make_num temporary) across success and early-return paths. It also adds a Linux-only RSS-growth regression gate to catch request-path leaks that LeakSanitizer cannot observe due to the HTTP test server being terminated via kill (no graceful atexit path).
Changes:
- Release JSON-parsed
Values and other borrowed temporaries inbuiltin_shared_incr,builtin_http_post, and thehttp_route_authedshared-store auth path. - Add
tests/test_http_rss_growth.shand wire it into the main test runner as suite[45c]to detect steady-state RSS growth on release builds. - Document the fix and the new leak gate in
CHANGELOG.mdand.claude/rules/c-runtime-memory.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/ext_http.c |
Adds missing val_decref calls and avoids leaking a temporary Value passed to eigs_json_encode. |
tests/test_http_rss_growth.sh |
Introduces a steady-state RSS growth gate for per-request leaks in the HTTP server. |
tests/run_all_tests.sh |
Integrates the new RSS-growth gate into the test suite as [45c]. |
CHANGELOG.md |
Documents the leak fixes and the rationale for the RSS-growth gate. |
.claude/rules/c-runtime-memory.md |
Updates contributor guidance on leak detection for ext_http.c (RSS gate + ownership rules). |
Comments suppressed due to low confidence (1)
tests/test_http_rss_growth.sh:128
- The RSS2 authed-route check also relies on a single randomly selected port (PORT_A). If that port is occupied, the check reports a failure even though the code under test is fine. Consider adding the same retry-on-port-collision behavior used in run_growth_check (or refactoring RSS2 to reuse run_growth_check) to keep this gate from being flaky.
PORT_A=$(( (RANDOM % 10000) + 52000 ))
AUTH_SRV=$(mktemp /tmp/eigs_rss_auth_XXXXXX.eigs)
cat > "$AUTH_SRV" <<EIGS
a is http_route of ["GET", "/asetup", "code", "shared_set of [\"require_auth\", \"\\\\\"\\\\\"\"]\n\"ok\""]
s2 is http_route_authed of ["GET", "/secret", "code", "\"top secret\""]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+87
to
+91
| if [ "$tries" -gt 100 ] || ! kill -0 "$srv_pid" 2>/dev/null; then | ||
| fail "$label server never came up" "port $port" | ||
| kill "$srv_pid" 2>/dev/null || true | ||
| rm -f "$srv_file" | ||
| return |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause — three sites, not one
#731 reported
shared_incr. I audited everyeigs_json_parse_valuecall sitein
ext_http.c; three of five leaked, two of them unreported:shared_incrmake_numhanded toeigs_json_encode(which borrows, not takes)builtin_http_posthttp_route_authedshared-store auth branch (~:1170)require_authnever releasedThe last two came from chasing the class rather than the reported instance.
Both are per-request leaks on the same footing as #731 — the authenticated-route
one arguably worse, since it fires on every request to a protected route.
shared_gettransfers ownership to its caller and the:508encode borrows;both are correct and untouched, so this isn't a blanket rule misapplied.
In
http_postthe release goes immediately after the header loop rather than atfunction end, because the
pipe/forkfailure paths below it return throughTRACE_NONDET_RECORD.The gate — and why it isn't a sanitizer
tests/test_http_rss_growth.sh, wired in as suite [45c].No sanitizer can catch this class here. LeakSanitizer reports from an
atexithandler;
test_http_server.shends withkill, andext_http.cinstalls noSIGTERM handler. The ASan suite reported
32 passed, 0 failedover this leakfor as long as it existed. So the gate measures RSS growth between two
steady-state checkpoints — never baseline-to-end, since the first requests
against a fresh server carry ~1.4 MB of one-time arena warmup, roughly 18x the
real leak rate.
Validated by planting the fault back rather than assuming the threshold works:
The gate skips on sanitizer builds, and that is load-bearing. I checked it
under
asan-httpbefore trusting it: ASan's redzones and quarantine grow RSS567 B/req on a fixed binary. Left unguarded it would have turned the
asan-httpCI job red on correct code. The suite prints the skip reasoninstead of
PASS: all 0 checks, so a gate that never ran cannot read as coverage.Requests are driven with curl
[1-N]globbing — one process, ~1.1s per 500 —so the gate costs a few seconds.
Validation
make http→ 3350/3350, gate active.make asan-http+detect_leaks=1→ 3352/3352, gate skipped for thedocumented reason.
(fixed binary measures exactly 0).
Not done, deliberately
Giving the server a graceful SIGTERM shutdown — #752's other option — would
let LSan cover this class directly instead of by proxy. That changes the server's
shutdown path with worker threads in flight; it's a real change on its own
merits, not something to fold into a leak fix. Worth filing separately if you
want it.
Closes #731
Closes #752